Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

#8174: [WIP] Replace ttlib custom Falcon matmuls with ttnn matmuls #8273

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

#8174: [WIP] Replace ttlib custom Falcon matmuls with ttnn matmuls #8273

wants to merge 1 commit into from

Conversation

TT-BrianLiu
Copy link
Contributor

No description provided.

@TT-BrianLiu
Copy link
Contributor Author

I am still debugging PCC drop from the switches and also need to make sure perf isn't impacted, but I want to get some early feedback from Falcon7B model owners.

@s-jovic
Copy link
Contributor

s-jovic commented May 9, 2024 •
edited
Loading

Thanks for doing the cleanup! I guess the main diff is that ttnn and legacy falcon cpp functions generate program config in a different way? Prefill-side, I think this should be ok since we already set explicit program config for perf-impactful matmuls.

One note though, there are two paths for prefill currently - optimized and the currently used one. I am preparing a PR for today that will switch to using this optimized version for relevant seq lens (128, 1k and 2k), so it might be good to see the pcc/perf once the switch is made.

@TT-BrianLiu
Copy link
Contributor Author

Thanks for doing the cleanup! I guess the main diff is that ttnn and legacy falcon cpp functions generate program config in a different way? Prefill-side, I think this should be ok since we already set explicit program config for perf-impactful matmuls.

One note though, there are two paths for prefill currently - optimized and the currently used one. I am preparing a PR for today that will switch to using this optimized version for relevant seq lens (128, 1k and 2k), so it might be good to see the pcc/perf once the switch is made.

Sounds good. I will rebase and test after your changes.

skhorasganiTT
skhorasganiTT approved these changes May 9, 2024
View reviewed changes
Copy link
Contributor

@skhorasganiTT skhorasganiTT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good, thanks for doing this Brian. FYI @tt-asaigal, matmuls have been changed to ttnn, @TT-BrianLiu will run the multi-chip tests to verify async performance

@TT-BrianLiu
Copy link
Contributor Author

Passing pipelines:

@skhorasganiTT skhorasganiTT mentioned this pull request May 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbradelTT bbradelTT bbradelTT approved these changes

@skhorasganiTT skhorasganiTT skhorasganiTT approved these changes

@eyonland eyonland Awaiting requested review from eyonland

@arakhmati arakhmati Awaiting requested review from arakhmati

@cfjchu cfjchu Awaiting requested review from cfjchu cfjchu is a code owner

@xanderchin xanderchin Awaiting requested review from xanderchin

@tt-aho tt-aho Awaiting requested review from tt-aho

@mywoodstock mywoodstock Awaiting requested review from mywoodstock

@djordje-tt djordje-tt Awaiting requested review from djordje-tt

@s-jovic s-jovic Awaiting requested review from s-jovic

At least 0 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@TT-BrianLiu @s-jovic @skhorasganiTT @bbradelTT